Remove redundant tests from Task SDK#68504
Open
shahar1 wants to merge 1 commit into
Open
Conversation
eladkal
added
the
changelog:skip
shahar1
force-pushed
the
remove-redundant-task-sdk-tests
branch
from
9e28a55 to
996b89e
Compare
Lee-W
reviewed
Jul 2, 2026
Lee-W
left a comment
Lee-W
left a comment
Member
There was a problem hiding this comment.
I feel we should keep P1 cases. We've been bitten by lib from time to time. P2 might be able to be removed in some cases, but it also ensure the value is not changed during object creation. totally agree with P5
related: apache#68502 These tests are exact or near-exact duplicates of sibling tests that already assert the same behavior, so they add maintenance cost without adding coverage: - test_mask_secret_with_list / test_mask_secret_with_iterable duplicate each other and are subsumed by the parametrized test_mask_secret_with_objects. - test_build_task_group_with_prefix_functionality duplicates test_build_task_group_with_prefix. - test_override_dag_default_args is byte-identical to test_default_args. Tests that guard against a library or attrs-generated behavior change (equality, hashing, repr, exception-hierarchy catches, attribute round-trips) are kept, per review feedback. Generated-by: Claude Code (Opus 4.8)
shahar1
force-pushed
the
remove-redundant-task-sdk-tests
branch
from
a3eec60 to
3d26f48
Compare
SameerMesiah97
approved these changes
Jul 2, 2026
SameerMesiah97
left a comment
SameerMesiah97
left a comment
Contributor
There was a problem hiding this comment.
I checked each of the test files you are removing these tests from and can confirm all 4 are redundant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
related: #68502.
This PR removes tests that are exact or near-exact duplicates of sibling tests
already asserting the same behavior — they add maintenance cost without adding
coverage.
Following review feedback, tests that guard against a library or attrs-generated
behavior change (equality, hashing, repr, exception-hierarchy catches, attribute
round-trips) are kept; only true duplicates are removed. Summarizing table is in
the "AI Summary".
AI Summary
Click Here
Legend — patterns:
test_build_task_group_with_prefix_functionalitytest_build_task_group_with_prefix: same Dag structure, same 7 assertions, only the Dag name string and inline comments differtest_override_dag_default_argstest_default_args(same Dag, sameTaskGroupdefault_args, same three owner assertions); only the name and docstring differtest_mask_secret_with_listtest_mask_secret_with_objects, which already covers a list input alongside dict and string casestest_mask_secret_with_iterabletest_mask_secret_with_list: identical setupexample_dict = ["test"]and identical assertion — claims to test an iterable but uses the same listWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines